-
Notifications
You must be signed in to change notification settings - Fork 106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cumulus 748 #409
Cumulus 748 #409
Conversation
We need to be sure there isn't an invalid package file left over from a previous aborted kes run - as the hash is generated and written as the filestream is opened in kes.utils.zip, and kes is capable of async calling process.exit, it's possible to have truncated/0 byte zip files. This validator ensures that the file is a legitimate zip archive in addition to a matching hash filename.
This is a fairly large number, however that seems to also be the case on master
Ratchet updates were incredibly large given the size of this PR, however the difference is also in master.... |
packages/deployment/lib/lambda.js
Outdated
return Promise.resolve(lambda); | ||
} | ||
catch (e) { | ||
console.log(`${lambda.local} appears to be an invalid zip file, and will be re-built`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually retry it if it's invalid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@laurenfrederick yes, the original code was skipping regeneration on L69, if the validator throws an error it logs and continues instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
fs.writeFileSync(existingLambdaLocal, 'hello'); | ||
t.is(fs.statSync(existingLambdaLocal).size, 5); | ||
fs.copySync(`${t.context.fixturedir}/zipfile-fixture.zip`, existingLambdaLocal); | ||
t.is(fs.statSync(existingLambdaLocal).size, 180); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the 180 is the size of zipfile-fixture.zip, would it make sense to do a
let origSize = fs.statSync(`${t.context.fixturedir}`/zipfile-fixture.zip).size;
(or something)? While it does add another line, it would make the test a bit more resilient to changes to the zip file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ifestus that's .... probably preferable for a fixture like this, as the contents are largely irrelevant, and it eliminates the magic-numbery feeling. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
packages/deployment/lib/lambda.js
Outdated
validateZipFile(lambda) { | ||
return new Promise((resolve, reject) => { | ||
yauzl.open(lambda.local, (err, _zipfile) => { | ||
if (err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be:
if (err) return reject(err);
return resolve();
If you don't return the rejection, it will continue on in the function and call resolve()
as well. Speaking from experience, that's an annoying bug to track down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, if you add const util = require('util');
to the top of this file, this entire function can be rewritten as:
return (util.promisify(yauzl.open))(lambda.local);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yjpa7145 Nice, I like that far better. TIL. Thanks!
packages/deployment/lib/lambda.js
Outdated
if (lambda.useMessageAdapter) { | ||
const kesFolder = path.join(this.config.kesFolder, 'build', 'adapter'); | ||
fileList.push(kesFolder); | ||
msg += ' and injecting message adapter'; | ||
} | ||
|
||
console.log(`${msg} for ${lambda.name}`); | ||
|
||
return utils.zip(lambda.local, fileList).then(() => lambda); | ||
return utils.zip(lambda.local, fileList).then(() => lambda).catch((e) => console.log(`Error zipping ${e}`)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in chat, please put the .then()
and .catch()
statements on their own lines, and re-throw the exception in the catch block after logging it.
@@ -62,7 +62,7 @@ test.serial('zipLambda: works for lambda not using message adapter', async (t) = | |||
t.context.lambda.useMessageAdapter = false; | |||
const lambdaLocalOrigin = t.context.lambda.local; | |||
const lambdaRemoteOrigin = t.context.lambda.remote; | |||
const l = new Lambda(t.context.config) | |||
const l = new Lambda(t.context.config); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be given a more descriptive variable name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I don't think it's possible to have a less descriptive variable name. How about testLambda.
@@ -87,7 +87,42 @@ test.serial('zipLambda: works for lambda using message adapter', async (t) => { | |||
}); | |||
|
|||
test.serial( | |||
`zipLambda: for lambda using message adapter, no new file is generated | |||
`zipLambda: given an invalid zip file generated from a previous run, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If string interpolation is not being performed, use single quotes instead of backticks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no interpolation, however it's a multi-line string due to line length. Do we have a project-standard (aside eslint) when it comes to that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the case of long, single-line strings, the project convention is to use single quotes. If eslint complains about the length of the line, add // eslint-disable-line max-len
to the end of the line.
`${Lambda.messageAdapterZipFileHash}-${path.basename(t.context.lambda.local)}` | ||
); | ||
|
||
fs.writeFileSync(existingLambdaLocal, 'hello'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of fs.writeFileSync
, look at using the fs-extra module and await fs.writeFile
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yjpa7145 out of curiosity, what is the benefit of doing an await fs.writeFile
over fs.writeFileSync
when on the next line the result of writeFile
is being referenced?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I think I see. If the next line is also async, then everything makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ifestus If you use the Sync call, the entire process stops until that writeFile operation finishes. That means that any other background operations (any asynchronous code running in the process) will hang until this write operation finishes. If the async version is used then those other asynchronous operations can continue while this write is being performed.
); | ||
|
||
fs.writeFileSync(existingLambdaLocal, 'hello'); | ||
t.is(fs.statSync(existingLambdaLocal).size, 5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above, consider using the Promise-returning version from fs-extra.
@@ -87,7 +87,42 @@ test.serial('zipLambda: works for lambda using message adapter', async (t) => { | |||
}); | |||
|
|||
test.serial( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason that these tests can't be run in parallel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Probably not, the setup function tempdir should be unique, it doesn't appear anything here should collide, assuming I'm not missing anything in the underlying dependencies.... why was it implemented that way initially 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running them in parallel results in several failures. I can dig into that further and put up another PR if appropriate, or address it here if it feels like it should be sorted in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like it's a defensive thing due to localstack constraints as much as anything - see #416
|
||
|
||
test.serial( | ||
`zipLambda: for lambda using message adapter, no new file is generated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about using single quotes instead of backticks.
@@ -103,20 +138,20 @@ test.serial( | |||
`${Lambda.messageAdapterZipFileHash}-${path.basename(t.context.lambda.remote)}` | |||
); | |||
|
|||
fs.writeFileSync(existingLambdaLocal, 'hello'); | |||
t.is(fs.statSync(existingLambdaLocal).size, 5); | |||
fs.copySync(`${t.context.fixturedir}/zipfile-fixture.zip`, existingLambdaLocal); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about using fs-extra and not Sync version.
|
||
const l = new Lambda(t.context.config) | ||
const l = new Lambda(t.context.config); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more descriptive variable name would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, a few requested changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have anything to add on top of Marc said. Great work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. It looks like the changes proposed in previous reviews have been addressed.
const fs = require('fs-extra'); | ||
const os = require('os'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Alphabetical
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a PR of my own against your CUMULUS-748 PR with the last changes that I recommend. I’m going to mark your PR as approved, your discretion as to whether you want to pull my PR into yours first.
My PR - #422
Marc's final feedback on CUMULUS-748
Summary: Resolves deployment issue where truncated lambda .zip files were being pushed out to AWS
Addresses [CUMULUS-748]
Changes
by filename hash alone
Test Plan
Things that should succeed before merging.
./bin/eslint-ratchet
and verify that eslint errors have not increased.eslint-ratchet-high-water-mark
if the score has improvedRelease PR
If this is a release PR, make sure you branch name starts with
release-
prefix, e.g.release-v-1.1.2
.Reviewers: @tag-your-friends